-
-
Notifications
You must be signed in to change notification settings - Fork 977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add shorter/simpler weather condition options. #2134
base: main
Are you sure you want to change the base?
Add shorter/simpler weather condition options. #2134
Conversation
Build size and comparison to main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this!
28be2dc
to
94748eb
Compare
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudShowerHeavy: | ||
return "Rain"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::CloudSunRain: | ||
return "Drizzle"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me drizzle is quite different from rain showers, it implies fine droplets at a low intensity whereas showers can be heavy but brief (more importantly one justifies an umbrella and the other might not!). I think "Showers" might fit better or just "Rain" even
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, "Drizzle" should be changed to "Rain" in my opinion. I'm not a huge fan of "Shower" for the heavier rain case as I feel like that could be misinterpreted as a light shower as well as a heavy shower... perhaps we could change the GetCondition return to "Downpour" for CloudsShowerHeavy, and make it "Pour" for GetSimpleCondition? Just a suggestion, not sure how everyone would interpret "Pour" though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These conditions are determined directly from the OpenWeather condition codes (which is also what Gadgetbridge uses). The actual API has a higher level of precision (light drizzle vs heavy shower and so on), but the weather apps compatible with Gadgetbridge for the most part only support a handful of the condition codes in the spec.
I would not recommend deviating from this spec, as it would compound the delta between the weather app and gadgetbridge...
94748eb
to
bcc7ca7
Compare
case Pinetime::Controllers::SimpleWeatherService::Icons::Snow: | ||
return "Snow"; | ||
case Pinetime::Controllers::SimpleWeatherService::Icons::Smog: | ||
return "Mist"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ("Mist"
) is the only questionable one. Based on the spec it seems like it should apply to anything that opacifies the atmosphere. However, most apps (and the spec icon itself) just rely on "mist" as a catch-all, so I guess it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me and I like the simplified descriptions (plus they match the spec), but where are we expecting to call GetSimpleCondition
? Are there existing or planned use cases for it? In my mind if you want a shorter condition, you can just get the icon.
bcc7ca7
to
1b4c981
Compare
These changes were originally created in PR #2001, but I'm spliiting them into their own PR as I believe they may be useful in other places and could be merged before the terminal watch face changes.
This change simply adds a
GetSimpleCondition
option as an alternative toGetCondition
to return a shorter condition description. For example: returning 'Thunder' instead of 'Thunderstorm' as a way to save space in constrained areas without losing the original descriptions meaning.There's an example GIF here of where this would be useful.